Skip to content

fix: use old type name in Union migration type signatures for renamed types#81

Open
CharlonTank wants to merge 3 commits intolamdera:lamdera-nextfrom
CharlonTank:fix/migration-union-type-rename
Open

fix: use old type name in Union migration type signatures for renamed types#81
CharlonTank wants to merge 3 commits intolamdera:lamdera-nextfrom
CharlonTank:fix/migration-union-type-rename

Conversation

@CharlonTank
Copy link
Copy Markdown
Contributor

@CharlonTank CharlonTank commented Mar 19, 2026

Summary

  • Fixes migration generator producing incorrect type signatures when a Union type is renamed between versions
  • The old type name is now correctly threaded through migrateUnionDefinition → migrateUnionDefinition_, mirroring how migrateAliasDefinition already handles alias renames

Problem

When a Union type is renamed between versions (e.g. MoralType in V51 → CompanyType in V52), the migration generator was using the new type name for the old version in the generated type signature:

migrate_LegalEntity_CompanyType : Evergreen.V51.LegalEntity.CompanyType -> Evergreen.V52.LegalEntity.CompanyType

But CompanyType doesn't exist in V51 — it was called MoralType. This causes a compilation error in the generated migration file.

Fix

The correct signature should be:

migrate_LegalEntity_CompanyType : Evergreen.V51.LegalEntity.MoralType -> Evergreen.V52.LegalEntity.CompanyType

Changes in MigrationGenerator.hs:

  1. coreTypeMigration: Pass typeNameOld (from pattern match) instead of typeName (new name) to migrateUnionDefinition
  2. migrateUnionDefinition: Renamed parameter from typeNameNew to typeNameOld, derive typeNameNew from the identifier's tipe component, and thread both to migrateUnionDefinition_
  3. migrateUnionDefinition_: Added typeNameOld parameter; use it in the type signature for the old version's type reference (line 279)
  4. migrateTypeDef: Pass typeNameOld instead of typeNameNew to migrateUnionDefinition

Reference

The Alias case (migrateAliasDefinition) already correctly passes and uses both typeNameOld and typeNameNew — this fix brings the Union case in line with that pattern.

Test plan

  • Rename a Union type between two Evergreen versions (e.g. type MoralType = ... → type CompanyType = ...)
  • Run lamdera check and verify the generated migration file uses the old type name for the old version in the type signature
  • Verify the generated migration file compiles without errors

@CharlonTank @claude
fix: use old type name in Union migration type signatures for renamed…
9a6d3e3 
… types

When a Union type is renamed between versions (e.g. MoralType in V51 →
CompanyType in V52), the migration generator was using the NEW type name
for the OLD version in the generated type signature:

  migrate_LegalEntity_CompanyType : Evergreen.V51.LegalEntity.CompanyType -> ...

But CompanyType doesn't exist in V51 - it was called MoralType. The correct
signature should be:

  migrate_LegalEntity_CompanyType : Evergreen.V51.LegalEntity.MoralType -> ...

The fix threads typeNameOld through migrateUnionDefinition →
migrateUnionDefinition_, mirroring how migrateAliasDefinition already
correctly handles this case for type alias renames.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CharlonTank CharlonTank mentioned this pull request Mar 19, 2026
Open
@CharlonTank
Copy link
Copy Markdown
Contributor Author

CharlonTank commented Mar 19, 2026

I built the fixed compiler locally and tested it against my project which has a Union type rename (MoralType → CompanyType between V51 and V52).

Before the fix:

migrate_LegalEntity_CompanyType : Evergreen.V51.LegalEntity.CompanyType -> Evergreen.V52.LegalEntity.CompanyType

→ Compilation error: Evergreen.V51.LegalEntity.CompanyType doesn't exist

After the fix:

migrate_LegalEntity_CompanyType : Evergreen.V51.LegalEntity.MoralType -> Evergreen.V52.LegalEntity.CompanyType

→ Correct type signature, compiles fine

@CharlonTank
fix: wrap pipeline migrations in lambda for nested Dict/SeqDict
a4605ae 
When a migration for a container type parameter is a pipeline
(e.g. nested SeqDict producing `SeqDict.toList |> List.map ... |> SeqDict.fromList`),
it must be wrapped in a lambda before being passed as a function argument
to Tuple.mapBoth, Tuple.mapFirst, List.map, etc.

Without this fix, the generated code produces:
  Tuple.mapBoth (migrate_Email) (SeqDict.toList |> ...)
which is syntactically invalid (pipeline used as a value argument).

With this fix:
  Tuple.mapBoth (migrate_Email) (\v__ -> v__ |> SeqDict.toList |> ...)
@CharlonTank
Copy link
Copy Markdown
Contributor Author

CharlonTank commented Mar 19, 2026

Added second fix: nested Dict/SeqDict pipeline wrapping

When a container type has a nested Dict/SeqDict parameter, the recursive migration produces a pipeline (SeqDict.toList |> ... |> SeqDict.fromList) that gets inserted directly as a function argument to Tuple.mapBoth, which is syntactically invalid.

Before: Tuple.mapBoth (migrate_Email) (SeqDict.toList |> List.map (...) |> SeqDict.fromList)
After: Tuple.mapBoth (migrate_Email) (\v__ -> v__ |> SeqDict.toList |> List.map (...) |> SeqDict.fromList)

Fix: In applyMigration, detect pipeline migrations (containing |>) and wrap them in a lambda — following the same pattern already used for anonymous records.

Both fixes tested locally — built the compiler and verified the generated migration compiles correctly.

@supermario
Copy link
Copy Markdown
Member

supermario commented Apr 25, 2026

@CharlonTank could you please add some test cases covering these changes?

@CharlonTank
test: add scenarios for Union rename and nested Dict pipeline fixes
ee65f89 
Migrate_Union_Renamed verifies the V_old type signature uses the old
type name when a Union is renamed across versions.

Migrate_Nested_Dict verifies that pipeline migrations passed as
arguments to Tuple.mapBoth are wrapped in a lambda.

Also tightens the pipeline-wrap heuristic to skip migrations that are
already parenthesised (e.g. tuple-destructuring lambdas), which was
double-wrapping legitimate cases like Migrate_All's UserListTriple.
@CharlonTank
Copy link
Copy Markdown
Contributor Author

CharlonTank commented Apr 27, 2026 •
edited
Loading

@supermario added two test scenarios in test/scenario-migration-generate/src/:

  • Migrate_Union_Renamed — covers fix number 1. Old has type MoralType, New has type CompanyType, both reachable via a Wrapper constructor in Target. Verified the test fails without the fix (Actual generates Old.CompanyType -> New.CompanyType, which doesn't exist in V_old) and passes with it.

  • Migrate_Nested_Dict — covers fix number 2. Target = Wrapper (Dict Int (Dict Int Int)) → Wrapper (Dict String (Dict String Int)), which forces both outer params to change and produces a nested pipeline as the second arg to Tuple.mapBoth. Without the fix, Actual generates Tuple.mapBoth (...) (Dict.toList |> ... |> Dict.fromList) (invalid as a function argument); with the fix it generates the (\v__ -> v__ |> ...) wrapper.

While adding these, I noticed fix number 2's "|>" \isInfixOf`check was too broad — it double-wrapped already-parenthesised lambdas likeMigrate_All's UserListTriple (((t1,t2,t3) -> (t1, t2, t3 |> migrate_X))), causing that existing test to fail. Tightened the check to skip migrations that already start with (`. All 7 tests pass now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@CharlonTank @supermario